-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(store): add field index to Store_SpliceDynamicData event #2279
Conversation
🦋 Changeset detectedLatest commit: 81be743 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| `start` | `uint48` | The start position in bytes for the splice operation. | | ||
| `deleteCount` | `uint40` | The number of bytes to delete in the splice operation. | | ||
| `encodedLengths` | `PackedCounter` | The encoded lengths of the dynamic data of the record. | | ||
| `data` | `bytes` | The data to insert into the dynamic data of the record at the start byte. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this all laid out, I am wondering:
- Do we still like the
Store_SpliceDynamicData
name since splice events are now ~guaranteed to operate on one field at a time and it includes both information for splicing within the field and for all of the dynamic data? Wondering if this should be something likeStore_SpliceDynamicField
to clarify the intent. - With above, I am also wondering if it's worth clarifying some argument names like
startWithinField
tofieldStart
andstart
->dynamicDataStart
. - "dynamic" in
dynamicFieldIndex
might be redundant since this is aStore_SpliceDynamicData
event. Also worth clarifying if this is the field index in the list of fields for the whole schema or field index among dynamic fields (I think it's the former?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need startWithinField
? Couldn't that be computed from dynamicFieldIndex
, start
and encodedLengths
? I'd prefer adding that one additional field to the event than refactoring the event name and other field names. The current naming feels very intuitive if you're thinking of the data as one byte blob, while the Store_SpliceDynamicField
naming would be a little more intuitive if you're storing data in separate locations, but I don't there is a strong enough bias in either direction to change the name at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const fieldLengths = getFieldLengths(encodedLengths); // some sort of decoding function
const fieldStart = fieldLengths.slice(0, dynamicFieldIndex).reduce((sum, length) => sum + length, 0);
const startWithinField = start - fieldStart;
checks out ✅
Co-authored-by: Kevin Ingersoll <[email protected]>
Fixes #2222